fix(gates): read a terminal dialog reply as one multi-line turn - #510

BLOCKING

The new guard is an exact user_input == "" check, but human.py:88 only strips trailing newlines ("\n".join(lines).rstrip("\n")), not trailing whitespace. A buffer of spaces or tabs survives as a truthy string and slips past both new guards.

Traced end-to-end through handle_dialog on the tty path with only input() mocked:

• Typing " " then /send → execute_dialog_turn(user_message=" ") gets awaited and a DialogMessage(role='user', content=' ') is recorded — exactly the case the CHANGELOG claims is fixed.
• Typing " " then Ctrl-D → hit_eof=True but text is still truthy, so the if hit_eof and not text branch at line 776 ("the user is leaving, not pasting") never fires. The whitespace gets dispatched as a turn and then the dialog dismisses. The user pressed Ctrl-D to leave and got a paste submission instead.

This isn't cosmetic — engine/workflow.py splices result.messages verbatim into the agent's re-execution guidance, so the agent gets re-run believing the user replied with whitespace. Nothing is logged on either path. _is_dismiss already normalizes with .strip().lower() elsewhere in this file, so these two guards are the outliers.

Same fix needed at line 776 (if hit_eof and not text.strip():), the docstring at 765-766 should say "nothing but whitespace accumulated" instead of "nothing accumulated," and it's worth a regression test with side_effect=[" ", EOFError()] asserting execute_dialog_turn.assert_not_awaited().

BLOCKING

if prompt_text is None and sys.stdin.isatty(): is the single condition choosing between two very different readers, and mutation testing shows the suite doesn't care which half you delete. Ran tests/test_gates plus tests/test_engine/test_dialog_integration.py, tests/test_cli/test_markup_injection.py, and tests/test_engine/test_resume.py (138 tests):

• Drop the isatty() half → 138 passed. The multi-line reader would now activate under a pipe or in CI, which is the exact failure this gate exists to prevent.
• Drop the prompt_text is None half → also 138 passed. The yes/no confirmation prompt at line 351 would start requiring /send after "yes" on every interactive run.

The cause: lines 781-789 (the Prompt.ask fallback) never execute in this repo's test suite. Every pre-existing dialog test patches handler._get_user_input wholesale, and every new test runs with isatty=True. Coverage confirms it — Missing … 781-789. The human gate already has the mirror test for its own equivalent condition at tests/test_gates/test_human.py:679; this needs the same pair here.

@pytest.mark.asyncio
async def test_non_tty_main_turn_uses_the_single_line_prompt(self) -> None:
    """Off a tty the conversational turn must stay on Prompt.ask, never raw input()."""
    with (
        patch("conductor.gates.dialog.sys.stdin.isatty", return_value=False),
        patch("conductor.gates.dialog.Prompt.ask", side_effect=["piped answer", "done"]) as ask,
        patch("builtins.input", side_effect=AssertionError("must not read raw stdin")),
    ):
        ...
    assert ask.call_count == 2

@pytest.mark.asyncio
async def test_confirmation_prompt_stays_single_line_on_a_tty(self) -> None:
    """`prompt_text` is a yes/no question -- it must not require the /send sentinel."""
    with (
        patch("conductor.gates.dialog.sys.stdin.isatty", return_value=True),
        patch("conductor.gates.dialog.Prompt.ask", return_value="yes") as ask,
        patch("builtins.input", side_effect=AssertionError("must not read multi-line")),
    ):
        answer = await handler._get_user_input(prompt_text=styled("[bold]Continue?[/bold]"))
    assert answer == "yes"
    ask.assert_called_once()

RECOMMENDED

"Ctrl-C dismisses rather than propagating" isn't true, and I don't think the test can fail even if it were wrong. A real Ctrl-C can't reach the except (EOFError, KeyboardInterrupt) at dialog.py:774: CPython only runs signal handlers on the main thread, PEP 475 silently retries the interrupted read, and the daemon thread blocked in input() never sees the signal. asyncio.Runner on 3.12+ handles SIGINT by cancelling the main task instead, so what actually surfaces at the await is CancelledError — which isn't in the catch tuple, correctly.

Verified this by sending a real SIGINT to a child process blocked in read_multiline_lines on a pty: the await raised CancelledError and KeyboardInterrupt escaped asyncio.run. There's no SIGINT handler anywhere in src/conductor, so Ctrl-C still tears the run down exactly like before.

tests/test_gates/test_dialog.py:844 (test_ctrl_c_dismisses_rather_than_propagating) only passes because patch("builtins.input", side_effect=KeyboardInterrupt()) raises inside the worker thread — the one route a genuine SIGINT can't take. That's the kind of test that gets cited later as proof this works when it doesn't.

Anyone who reads this changelog line and presses Ctrl-C expecting to close the dialog and keep the run going will instead kill the run and lose the session.

Drop "and Ctrl-C dismisses rather than propagating" from this entry, and fix the same claim in docs/workflow-syntax.md:1201 — Ctrl-D submits or dismisses, Ctrl-C aborts the run like it does everywhere else. Keep the except clause itself (it's correct for exceptions the reader raises), just rename the test to something like test_reader_exception_dismisses_rather_than_crashing_the_dialog and note in its docstring that a real SIGINT goes to the main thread and never exercises this path.

RECOMMENDED

Two things wrong in this one sentence.

First, the non-tty path did change. The new if user_input == "": continue at dialog.py:282 sits above the tty branch in the shared loop, and Prompt.ask returns "" for a blank line with no default set — confirmed by reading PromptBase.process_response and running it directly. Piping ["", "done"] with isatty() false now gives turns dispatched: 0; on origin/main that blank line was recorded as a DialogMessage, emitted as a dialog_message event, and dispatched to provider.execute_dialog_turn with empty content. The new behavior is fine — the problem is this changelog is the contract for anyone driving a dialog from a pipe or CI, and it tells them nothing changed when their turn count and event stream just did.

Second, the web dashboard isn't "off a tty" in the sense this implies. handle_dialog returns via _web_handle_dialog at line 207, before _display_dialog_start and before the loop — it reaches neither reader, and it's always received whole multi-line messages over the WebSocket. Grouping it with "a pipe, CI" reads as if it hits the Prompt.ask fallback, and someone reading docs/workflow-syntax.md:1201 will conclude the dashboard chat box can't take a multi-line message.

The inline comment at dialog.py:283-284 ("User submitted nothing on a tty") repeats the same mistake and will mislead the next person who touches this code.

Suggested rewording for both the CHANGELOG and the docs bullet:

Off a tty — a pipe or CI — replies are still read one line at a time and /send has no effect; the one change on that path is that a blank line is now skipped instead of dispatched as an empty turn. The web dashboard is unaffected: it takes a separate path that already delivered each message whole.

And widen the code comment:

            if not user_input.strip():
                # Empty submission -- not a turn, and not dismissal either. On a
                # tty this is a bare sentinel line; off a tty it is a blank line
                # from the pipe, which Prompt.ask returns as "".
                continue

RECOMMENDED

The comment here is honest that this except StopIteration exists only to satisfy exhausted unittest.mock side_effect lists — which is exactly why I don't think it belongs in shipped code. It's load-bearing: removing it breaks test_pasted_block_is_one_user_prompt_with_newlines and test_eof_mid_paste_submits_content_not_dismissal with RuntimeError: StopIteration interacts badly with generators and cannot be raised into a Future.

Two problems with leaving it:

1. The comment's justification is narrower than what the code actually does. input() doesn't originate StopIteration, but it does relay whatever sys.stdin.readline() raises — confirmed with an iterator-backed sys.stdin where a plain input() call propagated it. read_multiline_lines is a plain function, so PEP 479 doesn't help here. Nothing in src/ or plugins/ swaps sys.stdin/input today, so there's no live trigger, but if one ever appears, a broken stdin source would get silently reinterpreted as "the user pressed Ctrl-D" and a truncated turn would go to the model with nothing logged.
2. It weakens the tests that exist. "The reader read past what the test supplied" — the exact bug class this PR is fixing — now passes as a clean submission instead of failing.

It also makes the new _read_multiline docstring's claim ("behavior is unchanged by the extraction") not quite accurate, since the pre-PR loop only caught EOFError.

# src/conductor/gates/human.py:77-81
        try:
            line = input()
        except EOFError:
            hit_eof = True
            break

And terminate the test doubles explicitly instead of relying on the catch:

# tests/test_gates/test_dialog.py:749
side_effect=["line one", "line two", "line three", "/send", "done", EOFError()],
# tests/test_gates/test_dialog.py:774
side_effect=["ticket text", EOFError(), "done", EOFError()],

Both produce identical assertions against a strict except EOFError:.

RECOMMENDED

AGENTS.md:161, added by this PR, says: "The two gates use different sentinels, so a caller passes the one it wants rather than relying on the default." A default parameter whose own documentation tells callers not to use it is compensating for a signature that shouldn't allow the wrong call in the first place.

Both production call sites already pass it explicitly (human.py:537, dialog.py:772). The default's only live users are four test call sites in tests/test_gates/test_human.py (766, 782, 794, 798), where it implicitly means "the human gate's sentinel" — exactly the ambiguity splitting the constant was supposed to remove.

The risk isn't hypothetical: read_multiline_lines is public, lives in a module dialog.py already imports from, and read_multiline_lines(console) is silently valid with no signal at compile time. It would quietly truncate a user's prose at any lone . line — data loss, not a crash.

def read_multiline_lines(
    console: MarkupFreeConsole, *, sentinel: str
) -> tuple[str, bool]:

Making it keyword-only also rules out a future (sentinel, console) transposition. The cost is two keyword additions at the production call sites and four in test_human.py, and AGENTS.md can then say "…so sentinel is required rather than defaulted" and mean it.

RECOMMENDED

Lines 54 and 61-62 both say "trailing blank lines are stripped," but the implementation is "\n".join(lines).rstrip("\n") (line 88), which strips trailing newline characters, not trailing blank lines. Checked directly:

['a', '', '']     -> 'a'        # empty trailing lines: stripped
['a', '   ', '']  -> 'a\n   '   # whitespace-only trailing line: survives

"Blank line" ordinarily includes whitespace-only lines, so this overstates the guarantee — and it's the same gap behind the blocking whitespace issue above. A maintainer who trusts this docstring won't think to add .strip() at a call site that needs it. _read_multiline's docstring at line 534 inherits the same wording.

Either fix the wording in both places:

Internal newlines are preserved; trailing empty lines are dropped, but a trailing line of whitespace is kept verbatim.

or leave the code as-is and just note that changing it to a real .strip()-style rstrip would also eat meaningful trailing indentation from a pasted code block — which is probably the right call, so the docstring should give way instead.

RECOMMENDED

Mutating if line.strip() == sentinel: to if line == sentinel: still leaves 138/138 tests passing — every existing test types the sentinel byte-exactly ("." at test_human.py:766, "/send" at :774 and test_dialog.py:749).

A trailing space after /send is a normal keystroke, and terminals and paste buffers add them routinely. Without .strip() here, that line would get silently swallowed into the message body and the user would sit at a prompt that never submits. .strip() predates this PR, but this PR is what makes a five-character typed sentinel the primary way to send a dialog turn, so it's carrying a lot more weight now than before.

Worth flagging the flip side while you're adding coverage: .strip() also means a /send line inside a pasted block (a chat log, a shell transcript) truncates the paste with no warning, and the rest gets eaten as the next turn. /send is a much safer choice than the human gate's ., so this isn't a design objection — but a one-line echo after submission ((sent 3 lines)) would let the user catch it immediately.

@pytest.mark.parametrize("typed", ["/send", " /send", "/send ", "\t/send  "])
def test_sentinel_tolerates_surrounding_whitespace(self, typed: str) -> None:
    with patch("builtins.input", side_effect=["body", typed, "unreachable"]):
        text, hit_eof = read_multiline_lines(MagicMock(), sentinel="/send")
    assert (text, hit_eof) == ("body", False)

The "unreachable" entry also proves the reader stopped at the sentinel instead of just running out of mock values.

RECOMMENDED

The banner template mixes + concatenation with adjacent-literal concatenation, and multiline_hint has to stay in sync with hint_args — one {} in the fragment per element in the tuple — with nothing enforcing that.

To be clear about what isn't broken: I rendered both branches and the output is correct today. The trailing " " glues cleanly to "Say ..." via adjacent-literal concatenation, so there's no double or missing space, and the placeholder counts line up.

The concern is the next edit. styled() raises IndexError on a missing argument and silently drops an extra one — both confirmed — and ty can't check a template assembled with +. Add a second {} to the hint without extending the tuple and you get an IndexError that only fires on the tty branch, which never runs in CI unless isatty is patched. Confirming the sentence is even correct today requires the same manual trace I just did, which is a lot of reader effort for not much information.

styled() splices a Text value in with its own spans re-anchored, so the hint can be pre-rendered and dropped into a fixed-arity template instead. Rendered and diffed against current output — byte-identical on both branches, ANSI spans included:

multiline_hint = (
    styled(
        " It can span multiple lines; send it with [bold]{}[/bold] on its own line.",
        DIALOG_SUBMIT_SENTINEL,
    )
    if sys.stdin.isatty()
    else Text("")
)

self.console.print(
    Panel(
        styled(
            "[bold]Agent '{}'[/bold] would like to discuss its output with you.\n"
            "[dim]Type your response below.{} Say [bold]done[/bold] or "
            "[bold]/done[/bold] when finished.[/dim]",
            agent.name,
            multiline_hint,
        ),
        title=Text.from_markup("[bold magenta]Dialog Mode[/bold magenta]"),
        border_style="magenta",
    )
)

Separately, sys.stdin.isatty() now appears both here and at line 768 and has to stay in agreement, or the banner advertises a keystroke the reader ignores — right now that's held together by a comment alone. A shared module-level _reads_multiline_turn() predicate would state the rule once, and the existing patch("conductor.gates.dialog.sys.stdin.isatty") tests keep working unchanged.

RECOMMENDED

The PR description says each new dialog test "was confirmed to fail against the unpatched gate, so they guard the behavior rather than merely describing it." I checked that by restoring origin/main's _get_user_input body onto DialogHandler via an autouse fixture (no repo files touched) and running TestDialogMultilineInput unmodified:

test_pasted_block_is_one_user_prompt_with_newlines   FAILED  <- genuine
test_eof_mid_paste_submits_content_not_dismissal     PASSED
test_ctrl_d_at_empty_prompt_dismisses                PASSED
test_ctrl_c_dismisses_rather_than_propagating        PASSED
test_web_path_unaffected_by_multiline                PASSED

(Excluding the two banner cases — that harness only reverts _get_user_input, and the [True] banner case does genuinely fail if the banner change is reverted separately.)

Why each one passes on old code: on origin/main the turn went through Prompt.ask → input(), so EOFError/KeyboardInterrupt already hit the pre-existing except (EOFError, KeyboardInterrupt): return None and produced the same dismissal. test_eof_mid_paste_submits_content_not_dismissal asserts user_msgs[0].content == "ticket text" and assert_awaited(), both true on old code since Prompt.ask reads exactly that one line. test_web_path_unaffected_by_multiline guards a branch that returns at line 207, before the loop containing read_multiline_lines is ever reached.

Only test_pasted_block_is_one_user_prompt_with_newlines currently holds the line on the actual fix. No production risk here — the risk is to the review record, since three of these are presented as proof and don't actually provide it.

For test_eof_mid_paste_submits_content_not_dismissal, the missing assertion is that the EOF after the text didn't dismiss:

assert result.user_dismissed is False
assert [m.content for m in result.messages if m.role == "user"] == ["ticket text", "done"]

For test_ctrl_d_at_empty_prompt_dismisses, assert that the new reader was actually entered (e.g. that read_multiline_lines was called), so it pins this reader specifically rather than any reader. For the two structural guards, reword the docstrings to say "behavior preserved by the extraction" instead of implying regression coverage, and adjust the claim in the PR description to match.